-
Notifications
You must be signed in to change notification settings - Fork 30
CPU and Max RSS Analysis tools #2100
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
Fixed bug memory values
Tests added
This comment was marked as resolved.
This comment was marked as resolved.
af6ee22
to
c5f8cb4
Compare
We'll get round to this when we can, but a little bit swamped at the moment! |
I've had a first look - The "chips" look good:
If nothing has finished the analysis view sticks in this state: I threw a fairly nasty workflow at it and noticed some anomalies: Cannot page through, cannot see tasks I know to have completed. |
Oliver recommended the best way to move data around cylc was to use a debug message. Those chips only only appear in dev mode and won't be visible to normal users. |
Using the supercomputer login node as just another computer. We use the term background to mean execution a job in bash without using a job runner like PBS or Slurm. E.g. Warning Irrelevant |
Debug is def the right level to send these messages at, however, I'm not sure we've got the UI to hide debug level messages yet. Orthogonal to this PR either way. |
src/utils/tasks.js
Outdated
// If memory value passed | ||
} else if (timingOption === 'maxRss') { | ||
if (value < 5000) { | ||
const kilobytes = value | ||
return kilobytes.toPrecision(3) + ' KB' | ||
} else if (value / 1024 < 1000) { | ||
const megabytes = value / 1024 | ||
return megabytes.toPrecision(3) + ' MB' | ||
} else { | ||
const gigabytes = value / 1048576 | ||
return gigabytes.toPrecision(3) + ' GB' | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO this ought to be in a different function as it's not a duration
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, I think the name of the function should be change. Originally, when we only had timing on the analysis view, this function displayed the timings in a sensible format.
It's ensuring the appropriate scale, for the appropriate unit is displayed on the Y-axis. Wouldn't make sense to have two functions doing that.
formatUnits()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
formatDuration
is used by other components such as the Tree and Table views, so it needs to stay.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure I get you?
The the function won't change. I'm just changing the name to better reflect it's purpose
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its purpose should remain as formatting durations. I would add a new function for formatting RSS. If you want you could also have a function like
function formatUnits (...) {
if (duration) formatDuration(...)
else formatRSS(...)
}
Remove reference to 'suite' Co-authored-by: Ronnie Dutta <[email protected]>
Co-authored-by: Ronnie Dutta <[email protected]>
src/utils/tasks.js
Outdated
// If memory value passed | ||
} else if (timingOption === 'maxRss') { | ||
if (value < 5000) { | ||
const kilobytes = value | ||
return kilobytes.toPrecision(3) + ' KB' | ||
} else if (value / 1024 < 1000) { | ||
const megabytes = value / 1024 | ||
return megabytes.toPrecision(3) + ' MB' | ||
} else { | ||
const gigabytes = value / 1048576 | ||
return gigabytes.toPrecision(3) + ' GB' | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
formatDuration
is used by other components such as the Tree and Table views, so it needs to stay.
{ title: `Min ${this.timingOption} time`, value: `min${upperFirst(this.timingOption)}Time` }, | ||
{ title: `Max ${this.timingOption} time`, value: `max${upperFirst(this.timingOption)}Time` }, | ||
{ title: `Mean ${formatChartLabels(this.timingOption)}`, value: `mean${upperFirst(getTimingOption(this.timingOption))}` }, | ||
{ title: `Median ${formatChartLabels(this.timingOption)}`, value: `median${upperFirst(getTimingOption(this.timingOption))}` }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Conciseness Co-authored-by: Ronnie Dutta <[email protected]>
# Conflicts: # src/views/Analysis.vue
This apart of 3 pull requests for adding CPU time and Max RSS analysis to the Cylc UI.
This adds the Max RSS and CPU time (as measured by cgroups) to the table view, box plot and time series views.
Linked to;
cylc/cylc-flow#6663
cylc/cylc-uiserver#675
Check List
CONTRIBUTING.md
and added my name as a Code Contributor.setup.cfg
(andconda-environment.yml
if present).?.?.x
branch.